Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

polish #1597

Merged
merged 4 commits into from
Dec 25, 2024
Merged

polish #1597

merged 4 commits into from
Dec 25, 2024

Conversation

rajadilipkolli
Copy link
Owner

@rajadilipkolli rajadilipkolli commented Dec 25, 2024

Summary by CodeRabbit

  • New Features

    • Added content type assertions for JSON responses in key API tests.
    • Introduced methods for converting DTOs to entities in ArticleDTO and CommentDTO.
    • Added a new method to delete articles by ID in the ArticleController.
    • Created a new integration test class for comprehensive coverage of article functionalities.
  • Bug Fixes

    • Improved validation of API responses by ensuring correct content type.
  • Chores

    • Updated the googleJavaFormat plugin version for enhanced formatting rules.
  • Refactor

    • Streamlined entity conversion logic in ArticleService to utilize new DTO methods.
    • Updated parameter types for article ID across various classes to use Long.

Copy link
Contributor

coderabbitai bot commented Dec 25, 2024

Walkthrough

This pull request encompasses changes across multiple files in a Java project, primarily focusing on enhancing data conversion methods and updating testing assertions. Modifications include adding conversion methods to the ArticleDTO and CommentDTO records, updating the ArticleService to utilize these methods, changing the artifact ID and plugin version in the Maven configuration, and improving HTTP response content type assertions in the RedisControllerTest class.

Changes

File Change Summary
boot-ultimate-redis/src/test/java/.../RedisControllerTest.java Added .hasContentType(MediaType.APPLICATION_JSON) assertions to test methods
jpa/boot-read-replica-postgresql/pom.xml Updated artifactId and name from boot-jpa-read-replica-postgresql to boot-read-replica-postgresql; updated googleJavaFormat plugin version from 1.24.0 to 1.25.2
jpa/boot-read-replica-postgresql/src/main/java/.../ArticleDTO.java Added convertToArticle() method to convert DTO to entity
jpa/boot-read-replica-postgresql/src/main/java/.../CommentDTO.java Added convertToComment() static method for DTO to entity conversion
jpa/boot-read-replica-postgresql/src/main/java/.../ArticleService.java Annotated class with @Transactional(readOnly = true); added findById(Long id) and deleteById(Long id) methods; removed previous conversion methods
jpa/boot-read-replica-postgresql/src/main/java/.../ArticleController.java Added deleteArticle method to handle article deletion by ID
jpa/boot-read-replica-postgresql/src/test/java/.../ArticleControllerIntTest.java Introduced integration test class for ArticleController with methods for finding and saving articles

Possibly related PRs

Suggested labels

component: mongodb-elasticsearch

Poem

🐰 A Rabbit's Ode to Code Refactor 🔧
With DTOs dancing, conversion so neat,
Entities transform, making logic complete.
Tests shine brighter, assertions precise,
Maven plugins update, everything nice.
Code hops forward, with elegance true! 🚀


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/domain/CommentDTO.java (1)

7-9: Add null-safety checks for robust conversion.

If there's a possibility that commentDTO is null or contains a null comment, consider adding appropriate null checks (e.g., Objects.requireNonNull) to prevent potential NullPointerExceptions or inadvertently saving null fields to the database.

jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/service/ArticleService.java (1)

12-12: Transactional scope clarity.

Marking the entire class with @Transactional(readOnly = true) is generally fine, but be sure that any override, such as the @Transactional on the saveArticle method, is well-understood to handle write operations properly. This ensures no confusion regarding transactional boundaries, especially in a service class that mixes read and write functionality.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 186b678 and 2137e67.

📒 Files selected for processing (5)
  • boot-ultimate-redis/src/test/java/com/example/ultimateredis/controller/RedisControllerTest.java (3 hunks)
  • jpa/boot-read-replica-postgresql/pom.xml (1 hunks)
  • jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/domain/ArticleDTO.java (1 hunks)
  • jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/domain/CommentDTO.java (1 hunks)
  • jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/service/ArticleService.java (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • jpa/boot-read-replica-postgresql/pom.xml
🔇 Additional comments (5)
boot-ultimate-redis/src/test/java/com/example/ultimateredis/controller/RedisControllerTest.java (3)

31-31: Verifying JSON response format is a good practice
Adding .hasContentType(MediaType.APPLICATION_JSON) ensures the response body is correctly formatted as JSON, improving the reliability of your test suite.


46-46: Maintaining consistent content type checks
This line continues the pattern of verifying JSON responses, which helps keep the tests consistent and avoids any regressions in content negotiation.


66-66: Ensuring JSON format even in expiry scenario
Confirming the content type in scenarios where data may be expired helps ensure consistent API behavior and payload format.

jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/domain/ArticleDTO.java (1)

14-25: Ensure non-null handling for commentDTOs list.

To avoid possible NullPointerExceptions, consider validating that commentDTOs is not null before iterating. For example:

if (commentDTOs != null) {
    commentDTOs.forEach(...);
}

Or enforce non-null through the record constructor.

jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/service/ArticleService.java (1)

27-27: Effective usage of the new DTO conversion method.

Using articleDTO.convertToArticle() simplifies and centralizes the conversion logic. This is a cleaner approach than having multiple conversion methods in ArticleService—well done on improving maintainability.

Copy link

github-actions bot commented Dec 25, 2024

Qodana Community for JVM

101 new problems were found

Inspection name Severity Problems
@NotNull/@Nullable problems 🔶 Warning 34
Unchecked warning 🔶 Warning 32
'equals()' method which does not check class of parameter 🔶 Warning 21
Nullability and data flow problems 🔶 Warning 3
Field can be local 🔶 Warning 3
Declaration has problems in Javadoc references 🔶 Warning 2
AutoCloseable used without 'try'-with-resources 🔶 Warning 1
Class is exposed outside of its visibility scope 🔶 Warning 1
Javadoc declaration problems 🔶 Warning 1
Mismatched query and update of collection 🔶 Warning 1
Optional.get() is called without isPresent() check 🔶 Warning 1
Commented out code ◽️ Notice 1

☁️ View the detailed Qodana report

Contact Qodana team

Contact us at [email protected]

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/controller/ArticleController.java (1)

38-49: Consider returning HTTP 204 No Content instead of 202 Accepted.

Currently, the method returns a 202 (Accepted) when an article is successfully deleted. While this is valid, a 204 (No Content) response is often used to indicate a successful deletion operation with no further content. You might consider using 204 to more precisely align with typical REST conventions. Alternatively, continuing to use 202 is acceptable if there is an expectation of asynchronous completion.

jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/controller/ArticleControllerIntTest.java (1)

57-103: Comprehensive multi-step creation and deletion test.

  1. The saveArticle test method effectively checks the created resource’s location and follow-up GET request to validate the persisted article.
  2. The final DELETE step ensures proper coverage of the new delete functionality.
  3. Consider adding an additional verification step after deletion (e.g., an assertion that a subsequent GET now returns 404) to confirm that the resource was indeed removed.
jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/service/ArticleService.java (1)

32-34: Expose consistent naming for find methods.

You have findArticleById (accepting an Integer) and findById (accepting a Long). Aligning parameter types and method names can help reduce confusion and unify the service API.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2137e67 and c996d81.

📒 Files selected for processing (4)
  • jpa/boot-read-replica-postgresql/pom.xml (2 hunks)
  • jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/controller/ArticleController.java (2 hunks)
  • jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/service/ArticleService.java (2 hunks)
  • jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/controller/ArticleControllerIntTest.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • jpa/boot-read-replica-postgresql/pom.xml
🔇 Additional comments (6)
jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/controller/ArticleController.java (1)

7-7: Import statement for DeleteMapping.

Good addition of the DeleteMapping annotation to handle HTTP DELETE requests. This is a clear and intuitive choice to define a dedicated route for article deletion.

jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/controller/ArticleControllerIntTest.java (2)

1-4: Excellent use of AssertJ for clearer test assertions.

The static imports and usage of expressive methods (like assertThat()) improve test readability and maintainability. Keep it up!


28-56: Thorough testing of article retrieval.

The findArticleById test thoroughly verifies both the HTTP status and the JSON response content. Good job asserting the presence, types, and expected values of article fields to ensure data integrity.

jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/service/ArticleService.java (3)

12-12: Smart choice to use @Transactional(readOnly = true).

Marking the entire service as read-only by default is a best practice for performance and safety. This helps prevent accidental writes in methods that are intended to be read-only.


27-27: Ensure robust null handling for DTO conversions.

Invoking articleDTO.convertToArticle() assumes a valid ArticleDTO. Consider adding null checks or constraints to guard against potential NPEs if untrusted inputs are a concern in your context.


36-39: Good implementation of new delete functionality.

The deleteById method is straightforward and uses Spring Data JPA’s built-in method effectively.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/service/ArticleService.java (1)

36-39: Consider adding error handling for non-existent IDs.

The method should handle cases where the article doesn't exist. Consider either:

  1. Returning a boolean to indicate success/failure
  2. Throwing a custom exception for non-existent articles
     @Transactional
-    public void deleteById(Long id) {
-        articleRepository.deleteById(id);
+    public boolean deleteById(Long id) {
+        if (articleRepository.existsById(id)) {
+            articleRepository.deleteById(id);
+            return true;
+        }
+        return false;
     }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c996d81 and 90c8676.

📒 Files selected for processing (3)
  • jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/controller/ArticleController.java (3 hunks)
  • jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/repository/ArticleRepository.java (1 hunks)
  • jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/service/ArticleService.java (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/controller/ArticleController.java
🔇 Additional comments (4)
jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/repository/ArticleRepository.java (1)

14-14: Correct alignment of parameter type with the entity's primary key

Changing the parameter type from Integer to Long aligns with the Article entity's primary key type. This ensures type consistency throughout the application and prevents potential runtime issues.

Use the following script to confirm that all references to findByArticleId now pass a Long parameter:

✅ Verification successful

Parameter type consistency verified for findByArticleId method

The verification confirms that:

  • The findByArticleId method in ArticleRepository is correctly defined with a Long parameter type
  • The only usage of this method is in ArticleService.findArticleById, which passes a Long parameter (id) to the repository method

Let's verify the Article entity's ID type to ensure complete consistency:


Type consistency verified across the entire Article domain

The verification confirms complete type consistency:

  • Article entity uses Long for its ID field
  • ArticleRepository.findByArticleId accepts a Long parameter
  • ArticleService.findArticleById uses Long parameter
  • ArticleController.findArticleById accepts Long path variable

The parameter type change from Integer to Long is correctly aligned with the entity's primary key type throughout the entire call chain.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all references pass a Long argument 
rg -A 3 -B 3 'findByArticleId'

Length of output: 1698


Script:

#!/bin/bash
# Find Article entity definition
ast-grep --pattern 'class Article {
  $$$
}'

Length of output: 46


Script:

#!/bin/bash
# Find Article entity definition using ripgrep
rg -A 10 "class Article" 

Length of output: 5885

jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/service/ArticleService.java (3)

12-12: LGTM! Good optimization for read operations.

The class-level @Transactional(readOnly = true) annotation is a good practice that optimizes database performance by defaulting all methods to read-only transactions unless explicitly marked otherwise.


27-30: LGTM! Good refactoring of conversion logic.

Moving the conversion logic to ArticleDTO improves encapsulation and makes the service class cleaner.


21-23: Verify the impact of ID type change from Integer to Long.

The parameter type change from Integer to Long is good for alignment with JPA best practices, but we should verify all callers have been updated.

✅ Verification successful

ID type change from Integer to Long is consistently implemented

Based on the verification results:

  • The Article entity uses Long id as the primary key type
  • The ArticleRepository's findByArticleId method uses Long articleId
  • The ArticleController's endpoint accepts Long id
  • The ArticleService's findArticleById method uses Long id
  • No Integer types were found in Article-related files

The change from Integer to Long has been properly implemented across all relevant components in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining Integer id usage in article-related files
rg -l "Integer.*id.*Article" 

# Search for findArticleById method calls
ast-grep --pattern 'findArticleById($expr)'

Length of output: 76


Script:

#!/bin/bash
# Let's try a more comprehensive search for method calls and Integer usage

# Search for findArticleById method calls with context
rg "findArticleById" -A 2 -B 2

# Search for Article-related files that might contain Integer type
fd -e java . | xargs rg "Integer.*Article|Article.*Integer" -l

# Search for repository method signature
ast-grep --pattern 'findByArticleId($expr)'

Length of output: 2523


Script:

#!/bin/bash
# Let's check the Article entity and repository for ID type
rg "class Article" -A 5 -B 5

# Check the repository method signature
rg "findByArticleId" -A 2 -B 2

# Look for any Integer type in Article-related files
rg "Integer" -A 2 -B 2 jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/

Length of output: 7435

Comment on lines +32 to +34
public Optional<Article> findById(Long id) {
return articleRepository.findById(id);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider returning ArticleDTO instead of Article entity.

Exposing JPA entities directly could lead to lazy loading issues if the entity is accessed outside the transaction boundary. Consider converting to DTO before returning.

-    public Optional<Article> findById(Long id) {
-        return articleRepository.findById(id);
+    public Optional<ArticleDTO> findById(Long id) {
+        return articleRepository.findById(id).map(this::convertToArticleDTO);

Committable suggestion skipped: line range outside the PR's diff.

@rajadilipkolli rajadilipkolli merged commit c4b3134 into main Dec 25, 2024
8 of 10 checks passed
@rajadilipkolli rajadilipkolli deleted the polish branch December 25, 2024 17:45
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/controller/ArticleControllerIntTest.java (2)

20-21: Consider using @WebMvcTest for faster test execution.

The @SpringBootTest annotation loads the entire application context, which might be unnecessary if you're only testing the web layer. Consider using @WebMvcTest(ArticleController.class) for better test isolation and faster execution, unless you specifically need the full application context.


22-22: Add security and concurrent access tests.

Consider enhancing the test suite with:

  1. Security tests:

    • Authentication requirements
    • Authorization rules
    • CSRF protection
  2. Concurrent access tests:

    • Multiple simultaneous writes
    • Race conditions in update operations

Would you like me to provide example test cases for these scenarios?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 90c8676 and 8beedfa.

📒 Files selected for processing (2)
  • jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/controller/ArticleController.java (3 hunks)
  • jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/controller/ArticleControllerIntTest.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • jpa/boot-read-replica-postgresql/src/main/java/com/example/demo/readreplica/controller/ArticleController.java
🔇 Additional comments (1)
jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/controller/ArticleControllerIntTest.java (1)

24-24: Verify the custom MockMvcTester implementation.

The test uses a custom MockMvcTester instead of Spring's standard MockMvc. Let's verify this implementation to ensure it properly wraps Spring's MockMvc functionality.

✅ Verification successful

Let me gather more information about the MockMvcTester implementation.


Let me check one more thing to verify if this is Spring's official MockMvcTester.


MockMvcTester is Spring's official testing utility, not a custom implementation

The MockMvcTester being used is from Spring Framework's official testing library (org.springframework.test.web.servlet.assertj.MockMvcTester). It's not a custom implementation but rather Spring's AssertJ-style fluent API for testing MVC applications. This is a standard and well-tested component that properly wraps Spring's MockMvc functionality.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for MockMvcTester implementation
ast-grep --pattern 'class MockMvcTester {
  $$$
}'

# Search for MockMvcTester usage in other test files
rg -l "MockMvcTester"

Length of output: 699


Script:

#!/bin/bash
# Search for MockMvcTester implementation in AbstractIntegrationTest files
rg -A 20 "class MockMvcTester" scheduler/boot-scheduler-quartz/src/test/java/com/scheduler/quartz/common/AbstractIntegrationTest.java boot-ultimate-redis/src/test/java/com/example/ultimateredis/common/AbstractIntegrationTest.java jpa/boot-data-customsequence/src/test/java/com/example/custom/sequence/common/AbstractIntegrationTest.java batch-boot-jpa-sample/src/test/java/com/example/bootbatchjpa/common/AbstractIntegrationTest.java

# Search for MockMvc usage within MockMvcTester
rg "MockMvc" scheduler/boot-scheduler-quartz/src/test/java/com/scheduler/quartz/common/AbstractIntegrationTest.java boot-ultimate-redis/src/test/java/com/example/ultimateredis/common/AbstractIntegrationTest.java jpa/boot-data-customsequence/src/test/java/com/example/custom/sequence/common/AbstractIntegrationTest.java batch-boot-jpa-sample/src/test/java/com/example/bootbatchjpa/common/AbstractIntegrationTest.java

Length of output: 3849


Script:

#!/bin/bash
# Check the import statement in the ArticleControllerIntTest
rg -A 5 "import.*MockMvcTester" jpa/boot-read-replica-postgresql/src/test/java/com/example/demo/readreplica/controller/ArticleControllerIntTest.java

# Check if there's a custom implementation in the project
rg -g "*.java" -l "class MockMvcTester"

Length of output: 332

Comment on lines +28 to +55
@Test
void findArticleById() {

mvcTester
.get()
.uri("/articles/1")
.assertThat()
.hasStatusOk()
.hasContentType(MediaType.APPLICATION_JSON)
.bodyJson()
.convertTo(ArticleDTO.class)
.satisfies(
articleDTO -> {
assertThat(articleDTO.title())
.isNotNull()
.isEqualTo("Waiter! There is a bug in my JSoup!");
assertThat(articleDTO.authored())
.isNotNull()
.isInstanceOf(LocalDateTime.class);
assertThat(articleDTO.published())
.isNotNull()
.isInstanceOf(LocalDateTime.class);
assertThat(articleDTO.commentDTOs())
.isNotNull()
.hasSize(2)
.hasOnlyElementsOfType(CommentDTO.class);
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test coverage with setup and error scenarios.

The test makes assumptions about existing test data and lacks coverage for error scenarios. Consider these improvements:

  1. Add a @BeforeEach method to set up test data consistently
  2. Add tests for error scenarios (e.g., non-existent article)
  3. Move test data to constants or test properties file

Example structure:

private static final String TEST_ARTICLE_TITLE = "Waiter! There is a bug in my JSoup!";

@BeforeEach
void setUp() {
    // Setup test data
}

@Test
void findArticleById_NotFound() {
    mvcTester.get()
            .uri("/articles/999")
            .assertThat()
            .hasStatus(HttpStatus.NOT_FOUND);
}

Comment on lines +57 to +103
@Test
void saveArticleRetriveAndDelete() throws JsonProcessingException {
ArticleDTO articleDTO =
new ArticleDTO(
"junitTitle",
LocalDateTime.now().minusDays(1),
LocalDateTime.now(),
List.of(new CommentDTO("junitComment")));
AtomicReference<String> location = new AtomicReference<>();
mvcTester
.post()
.uri("/articles/")
.contentType(MediaType.APPLICATION_JSON)
.content(objectMapper.writeValueAsString(articleDTO))
.assertThat()
.hasStatus(HttpStatus.CREATED)
.matches(
result -> {
location.set(result.getResponse().getHeader("Location"));
assertThat(location.get()).isNotBlank().contains("/articles/");
});

mvcTester
.get()
.uri(location.get())
.assertThat()
.hasStatusOk()
.hasContentType(MediaType.APPLICATION_JSON)
.bodyJson()
.convertTo(ArticleDTO.class)
.satisfies(
response -> {
assertThat(response.title()).isNotNull().isEqualTo("junitTitle");
assertThat(response.authored())
.isNotNull()
.isInstanceOf(LocalDateTime.class);
assertThat(response.published())
.isNotNull()
.isInstanceOf(LocalDateTime.class);
assertThat(response.commentDTOs())
.isNotNull()
.hasSize(1)
.hasOnlyElementsOfType(CommentDTO.class);
});

mvcTester.delete().uri(location.get()).assertThat().hasStatus(HttpStatus.ACCEPTED);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Split the test method and add validation scenarios.

The current test method violates the Single Responsibility Principle by testing multiple operations. Consider these improvements:

  1. Split into separate test methods for better isolation:

    • testSaveArticle
    • testRetrieveArticle
    • testDeleteArticle
  2. Add validation scenario tests:

    • Invalid article data
    • Missing required fields
    • Invalid date ranges
  3. Fix potential timing issues with LocalDateTime.now()

Example structure:

@Test
void saveArticle_ValidData() {
    ArticleDTO articleDTO = createValidArticleDTO();
    // Test POST only
}

@Test
void saveArticle_InvalidData() {
    ArticleDTO articleDTO = createInvalidArticleDTO();
    mvcTester.post()
            .uri("/articles/")
            .contentType(MediaType.APPLICATION_JSON)
            .content(objectMapper.writeValueAsString(articleDTO))
            .assertThat()
            .hasStatus(HttpStatus.BAD_REQUEST);
}

private ArticleDTO createValidArticleDTO() {
    LocalDateTime now = LocalDateTime.now();
    return new ArticleDTO(
            "junitTitle",
            now.minusDays(1),
            now,
            List.of(new CommentDTO("junitComment")));
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant